[Fix] in Xnnpack EP, the conversion for fused activation param isn't correct #23115
+59
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
December 16, 2024 22:31
wejoncy
reviewed
Dec 17, 2024
| ? *reinterpret_cast<const float*>(value.raw_data().data()) | ||
| : value.float_data()[0]; | ||
| int32_t arg_type; | ||
| if (GetType(arg, arg_type) && arg_type == ONNX_NAMESPACE::TensorProto_DataType_FLOAT16) { |
There was a problem hiding this comment.
What if GetType(arg, arg_type) failed here?
There was a problem hiding this comment.
Generally type info is always available, so I think this is ok. Shape info may be missing depending on the model.
The Conv op looks to be setup to allow fp32, u8, s8 and optionally fp16. Should this also handle u8 and s8 or should ClipReluChecker limit fusion to fp32 and fp16?
skottmckay
reviewed
Dec 17, 2024
Comment on lines
+134
to
+135
| // So far, CPU EP doensn't support Fp16 Conv fusion, so verify_outputs is skipped. | ||
| RunAndVerifyOutputsWithEP(ort_model_path, "TestNhwcConvReluClipFusion_FP16", std::move(ep), feeds, params, {}, false); |
There was a problem hiding this comment.
Not quite following. There should still be valid output from the CPU EP even if it doesn't fuse, so why can't we use verify_outputs?
Suggested change
| // So far, CPU EP doensn't support Fp16 Conv fusion, so verify_outputs is skipped. | |
| RunAndVerifyOutputsWithEP(ort_model_path, "TestNhwcConvReluClipFusion_FP16", std::move(ep), feeds, params, {}, false); | |
| // So far, CPU EP doesn't support Fp16 Conv fusion, so verify_outputs is skipped. | |
| RunAndVerifyOutputsWithEP(ort_model_path, "TestNhwcConvReluClipFusion_FP16", std::move(ep), feeds, params, {}, false); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In Xnnpack EP, the activation_param's conversion isn't correct for Fp16 model
Sometimes, it may cause an exception that "lower bound must be below upper bound"
Because CPU EP doesn't support FP16 activation fusion now, so the newly added test skips the comparison of the test result.
Motivation and Context